GH-49102: [CI] Add type checking infrastructure and CI workflow for type annotations#48618
GH-49102: [CI] Add type checking infrastructure and CI workflow for type annotations#48618rok wants to merge 29 commits intoapache:mainfrom
Conversation
afe4699 to
30017ff
Compare
|
Yes, let's wait for Raul 👍 |
|
@github-actions crossbow submit wheel-manylinux-2-28-cp311-cp311-amd64 |
|
Revision: 28fba3a Submitted crossbow builds: ursacomputing/crossbow @ actions-8a459250fe
|
compose.yaml
Outdated
| ARROW_S3: "OFF" | ||
| ARROW_SUBSTRAIT: "OFF" | ||
| ARROW_WITH_OPENTELEMETRY: "OFF" | ||
| PYARROW_TEST_ANNOTATIONS: "ON" |
There was a problem hiding this comment.
@raulcd do you think we actually want to be able to toggle these? I'm thinking we don't really need them
There was a problem hiding this comment.
I think is worth it if instead of using a separate ci/scripts/python_test_type_annotations.sh we use ci/scripts/python_test.sh and we use the PYARROW_TEST_ANNOTATIONS variable to manage whether we want annotations to be tested or not as part of python testing.
If we decide we want to have two separate scripts I think this env var should be removed.
I lean towards having a single python test script that can allow us to also test annotations but I can find arguments for both so not an issue.
There was a problem hiding this comment.
Ok, agreed with your reasoning. I also introduced PYARROW_TEST_ANNOTATIONS to the windows script.
|
Thanks for review @raulcd ! I've responded to your points. |
compose.yaml
Outdated
| ARROW_S3: "OFF" | ||
| ARROW_SUBSTRAIT: "OFF" | ||
| ARROW_WITH_OPENTELEMETRY: "OFF" | ||
| PYARROW_TEST_ANNOTATIONS: "ON" |
There was a problem hiding this comment.
I think is worth it if instead of using a separate ci/scripts/python_test_type_annotations.sh we use ci/scripts/python_test.sh and we use the PYARROW_TEST_ANNOTATIONS variable to manage whether we want annotations to be tested or not as part of python testing.
If we decide we want to have two separate scripts I think this env var should be removed.
I lean towards having a single python test script that can allow us to also test annotations but I can find arguments for both so not an issue.
| # We first populate stub docstrings and then build the wheel | ||
| python setup.py build_ext --inplace | ||
| python -m pip install griffe libcst | ||
| python ../dev/update_stub_docstrings.py pyarrow-stubs |
There was a problem hiding this comment.
Same as #48618 (comment). It's a noop and we don't have to introduce python logic in stub PRs if we do here.
| @REM We first populate stub docstrings and then build the wheel | ||
| %PYTHON_CMD% setup.py build_ext --inplace | ||
| %PYTHON_CMD% -m pip install griffe libcst |
There was a problem hiding this comment.
Until we have real annotations this isn't necessary, right?
There was a problem hiding this comment.
I added running dev\update_stub_docstrings.py, so I suppose it's a noop now and we can keep it to make sure it doesn't fail before introducing actual stubs? This will keep CI logic out of stub PRs.
change bat lint add a popd and nicer logging for windows ReplaceElipsis -> DocstringInserter simplify remove sphinx
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
a88b174 to
02c83e0
Compare
|
I rebased on main to fix the docs CI issues. |
Rationale for this change
This is the first in series of PRs adding type annotations to pyarrow and resolving #32609.
What changes are included in this PR?
This PR establishes infrastructure for type checking:
py.typedmarker to enable type checkingAre these changes tested?
No. This is mostly a CI change.
Are there any user-facing changes?
This does not add any actual annotations (only
py.typedmarker) so user should not be affected.